API, control, netstack: error refactoring by nrc · Pull Request #154 · tailscale/tailscale-rs

nrc · 2026-04-29T02:31:22Z

An attempt at refactoring some of our errors into a more handling-oriented approach. I'm a bit unsure about some of the names, descriptions, and categorisations, so feedback on that stuff in particular (as well as the rest of the approach) would be welcome!

npry

Directionally I'm on board, I think the current shape has some awkwardness around the specific composition of structs and enums (mostly in ts_control). The main hangup is that I'm not sure why we're nesting enum-in-enum there. As I mention in one of my comments, it feels to me like it wants to be a struct Error with a flat ErrorKind enum, so I'm curious why you laid it out that way

nrc · 2026-04-29T23:57:45Z

Directionally I'm on board, I think the current shape has some awkwardness around the specific composition of structs and enums (mostly in ts_control).

Yeah, a few places did feel pretty awkward and I definitely think this can be improved.

The main hangup is that I'm not sure why we're nesting enum-in-enum there. As I mention in one of my comments, it feels to me like it wants to be a struct Error with a flat ErrorKind enum, so I'm curious why you laid it out that way

So the idea here is that the top-level variants for each variant reflect possible recovery strategies - e.g., in ts_control, MachineNotAuthorized is lifted out because there is a specific user action to fix it, Protocol is meant to be an 'expected' error, where the user might need to re-check some parameters and then retry, and Internal is something that can't reasonably be recovered from (i.e., the user could maybe retry once, then report to the end-user and abort). Within those, ProtocolPhase is meant to further narrow down the issue for better error recovery, and ErrorKind and ConnectionPhase are meant to be purely informational so that the error that might be reported or logged can be more informative than just 'Internal error: internal error'.

This could all be better documented! And some of the names can definitely be improved.

Hopefully that makes sense why there are nested enums and not a struct.

nrc · 2026-04-30T04:58:53Z

Latest commit makes some of the suggested changes. I haven't updated docs to correspond or applied suggestions which I'd like to discuss further.

Signed-off-by: Nick Cameron <nrc@ncameron.org>

nrc · 2026-05-04T02:35:29Z

Updated the PR following our chat last week. The changes are in a separate commit. I think I got everything we discussed (and comments here), but let me know if I missed anything. I'm much happier with the new errors.

There is one question inline (marked REVIEW) about some IO errors which I didn't know how to handle (either because they are likely to be network errors but not always or are network errors but retrying won't help (I think)).

There is also one question in a reply above because of my bad memory.

npry

bunch of small nits, but the blocker is that imo NetworkError in its various incarnations probably want to split along the lines of issues <= L4 and > L4, i.e. is the network itself down, or is this a higher-layer problem? the former can be handled by retries or possibly out-of-band online checks, where an http 400 or a tls negotiation failure can't be resolved the same way (not retryable). probably this latter case can be handled as an Internal variant for now?

edit: otherwise, lgtm!

Signed-off-by: Nick Cameron <nrc@ncameron.org>

nrc · 2026-05-04T21:33:57Z

@npry all comments addressed. Thank you for helping me iterate on this!

npry

github was having a fit when i was giving this a look yesterday, sorry for the delay

want to make sure we didn't get wires crossed about the NetworkError point -- i assume my comments here are just instances i didn't directly comment on, so you didn't update, rather than a disagreement about the concept? lmk if not, but lgtm assuming these are resolved

npry · 2026-05-07T07:12:11Z

+impl From<ts_http_util::Error> for RegistrationError {
+    fn from(error: ts_http_util::Error) -> Self {
+        tracing::error!(%error, "http error sending registration request");
+        RegistrationError::NetworkError
+    }


should only be network error if io? (which is unfortunately currently undifferentiated within ts_http_util)

npry · 2026-05-07T07:12:35Z

        tracing::error!(%body, %status, "registration failed");

-        return Err(RegistrationError::RegistrationFailed(status.as_u16()));
+        return Err(RegistrationError::NetworkError);


this is pretty specifically an http error (status code) / not network

npry · 2026-05-07T07:13:13Z

+    #[error("error during registration: {0}")]
+    Internal(InternalErrorKind),
+    #[error("HTTP error")]
+    NetworkError,


based on usage below i think this wants to be http error? or reconcile the #[error] message

npry · 2026-05-07T07:15:00Z

+    #[error("invalid URL: {0}")]
+    InvalidUrl(url::Url),
+
+    /// Some kind of networking error, e.g., HTTP, TLS.


specifically not http or tls?

npry · 2026-05-07T07:19:52Z

+impl From<ts_http_util::Error> for Error {
+    fn from(error: ts_http_util::Error) -> Self {
+        tracing::error!(%error, "http error");
+        Error::NetworkError(Operation::ConnectToControlServer)


? http shouldn't be NetworkError unless it's ts_http_util::Error::Io

npry · 2026-05-07T07:21:08Z

        let port = url
            .port_or_known_default()
-            .ok_or(ConnectionError::ConnectionFailed)?;
+            .ok_or(Error::InvalidUrl(url.clone()))?;


nit: ok_or_else to skip the clone on the happy path

npry · 2026-05-07T07:21:22Z

        "https" => {
            let conn = ts_tls_util::connect(
-                ts_tls_util::server_name(url).ok_or(ConnectionError::ConnectionFailed)?,
+                ts_tls_util::server_name(url).ok_or(Error::InvalidUrl(url.clone()))?,


npry · 2026-05-07T07:22:41Z

-    MissingPayload,
-    #[error(transparent)]
-    WatchRecv(#[from] watch::error::RecvError),
+    #[error("HTTP error")]


also reconcile here

npry · 2026-05-07T07:25:41Z

+        tracing::error!(%error, "Error parsing HTTP request");
+        PingError::NetworkError


which do you mean? this is inconsistent -- http parse wouldn't be a network error, and this From is also invoked for the http2 post, which could be L3 interruption or L7 failure

nrc requested review from danderson, dylan-tailscale and npry as code owners April 29, 2026 02:31

nrc force-pushed the nrc/errors branch 2 times, most recently from 5119783 to 4941d68 Compare April 29, 2026 02:53

npry requested changes Apr 29, 2026

View reviewed changes

nrc force-pushed the nrc/errors branch from d8e098f to c2c8f4f Compare April 30, 2026 05:06

nrc added 3 commits May 4, 2026 11:11

Refactor ts_control errors

ae6aa93

Signed-off-by: Nick Cameron <nrc@ncameron.org>

Refactor errors in netstack and API

61fb9f6

Signed-off-by: Nick Cameron <nrc@ncameron.org>

Update changelog

8c12371

Signed-off-by: Nick Cameron <nrc@ncameron.org>

nrc force-pushed the nrc/errors branch from c2c8f4f to ffc80c2 Compare May 4, 2026 02:29

nrc force-pushed the nrc/errors branch 2 times, most recently from 4ed2420 to afcf59f Compare May 4, 2026 02:43

npry requested changes May 4, 2026

View reviewed changes

Review changes

b790d8c

Signed-off-by: Nick Cameron <nrc@ncameron.org>

nrc force-pushed the nrc/errors branch from afcf59f to b790d8c Compare May 4, 2026 21:33

npry approved these changes May 7, 2026

View reviewed changes

		tracing::error!(%error, "Error parsing HTTP request");
		PingError::NetworkError

Conversation

nrc commented Apr 29, 2026

Uh oh!

npry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrc commented Apr 29, 2026

Uh oh!

nrc commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrc commented May 4, 2026

Uh oh!

npry left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nrc commented May 4, 2026

Uh oh!

npry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nrc commented Apr 30, 2026 •

edited

Loading

npry left a comment •

edited

Loading